Conversation
This comment was marked as spam.
This comment was marked as spam.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — the underlying diagnosis is right and worth fixing: baking target_datetime into start_trigger_args at parse time means the serialized Dag hash changes every day, and the hash-stability test you added is a good way to pin that.
But the fix is to remove start-from-trigger support from TimeSensor, and that's a bigger call than the PR framing suggests. Anyone using TimeSensor(start_from_trigger=True) today has their sensor start directly in the Triggerer; after this it warns, is ignored, and the task instead occupies a worker slot to poke or defer. That's a functional regression for those users, not just churn cleanup.
Concretely that needs: a major version bump on apache-airflow-providers-standard (currently 1.16.0) and an entry under the Changelog header in providers/standard/docs/changelog.rst spelling out the required user action. Neither is in the diff. (No newsfragment — providers don't use those.)
The design question I'd like your view on: was removal the only option? Computing moment lazily — so start_trigger_args serializes a stable placeholder and the concrete datetime is resolved when the trigger actually starts — would fix the hash churn while keeping the feature. If that was tried and doesn't work, saying why in the description would help; if it wasn't, it seems worth a look before dropping a documented capability.
One correction to something I'd have flagged: trigger_kwargs is already accepted-and-ignored on main (it's declared in __init__ but never read), so dropping it here changes nothing. Not an issue with this PR.
To be clear about where this stands: the points above are mostly design questions rather than defects, and I'd rather you formed your own view on them than took mine as settled. My review here was AI-assisted, and I'd guess parts of this PR were too — that's fine on both sides, but it means neither of us should treat the output as authoritative. Please push back where you think the reasoning is wrong, and say what you think the right trade-off is between fixing the hash churn and keeping the feature.
I'd also like other maintainers to weigh in before this is decided. Removing a documented capability from a released provider isn't a call I want to make on my own, and folks closer to the start-from-trigger design will have better context here than I do.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
01d6a74 to
46d316c
Compare
TimeSensor(start_from_trigger=True) had two defects: 1. __init__ mutated the class-level StartTriggerArgs.trigger_kwargs, so every TimeSensor shared the last-constructed sensor's moment. 2. That moment was an absolute datetime from datetime.now() at parse time, so each Dag-processor parse produced a different serialized blob and a new DagVersion. Preserve start_from_trigger by introducing TimeOfDayTrigger, which stores only parse-stable target_time + timezone and resolves the concrete moment when the trigger starts. TimeSensor builds a per-instance StartTriggerArgs (never mutates the class attribute). No major version bump: the public API is preserved. Fixes: apache#69543
The temporal trigger docstring broke the generated API docs page (bullet list without a preceding blank line). Pendulum's in_timezone rejects a generic tzinfo in its type stubs, so normalize to a pendulum zone at the boundary. The serialization-stability tests used LazyDeserializedDAG.from_dag, which does not exist on Airflow 2.11/3.0-3.2 - pick the serialization API available on the running version instead.
A class-level TimeOfDayTrigger template leaked dummy kwargs into every TimeSensor serialization, and start_from_trigger silently wrote UTC when no Dag was attached, hiding a missing timezone. The trigger parameter is named tz so it does not shadow the SDK timezone module.
Delegating to a fresh DateTimeTrigger and omitting the resolved moment from serialize() meant a reconstruct after midnight could wait another day. Resolve at construction and persist moment so the wait target does not move. Co-authored-by: Cursor <cursoragent@cursor.com>
a604a90 to
92fee51
Compare
I see there is another discussion relaated to how to solve it . I dismiss my review to unblock it.
This comment was marked as spam.
This comment was marked as spam.
uranusjr
left a comment
uranusjr
left a comment
There was a problem hiding this comment.
Although we could import the timezone encode/decode logic from SDK, I think it’s probably not be worst idea to have them directly inside the provider code. The important thing here is both sides must agree, and having them both in the same place is more important for the provider than reducing duplication from Airflow Core or SDK.
Only one minor suggestion on using replace instead of the class directly.
The class-level template is the parse-stable source, matching DateTimeSensor and the previous code on main. Rebuilding StartTriggerArgs by hand dropped that shared object and made two sensors look like they constructed args from scratch.
…-version-69543 Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
DayOfWeekSensor's xref in the standard provider's sensor guide pointed at the pre-provider-split airflow.sensors.weekday path, which no longer exists; the class now lives under airflow.providers.standard.sensors.weekday. BaseXCom's xref in the common.io XCom backend guide pointed at airflow.models.xcom.BaseXCom, which is a deprecated compat shim; the real class lives at airflow.sdk.bases.xcom.BaseXCom. The TimeSensor xref in the same sensor guide is also broken (points at a non-existent sensors.time_sensor module) but is left alone here since apache#69610, apache#69746, and apache#69925 are all open against that same paragraph.
DayOfWeekSensor's xref in the standard provider's sensor guide pointed at the pre-provider-split airflow.sensors.weekday path, which no longer exists; the class now lives under airflow.providers.standard.sensors.weekday. BaseXCom's xref in the common.io XCom backend guide pointed at airflow.models.xcom.BaseXCom, a deprecated compat shim. The task-sdk docs only expose it at the flat airflow.sdk.BaseXCom path (autoapiclass in task-sdk/docs/api.rst, autoapi_generate_api_docs=False elsewhere), matching the existing convention for this class of xref (e.g. airflow.sdk.ResumableJobMixin). The TimeSensor xref in the same sensor guide is also broken (points at a non-existent sensors.time_sensor module) but is left alone here since apache#69610, apache#69746, and apache#69925 are all open against that same paragraph.
DayOfWeekSensor's xref in the standard provider's sensor guide pointed at the pre-provider-split airflow.sensors.weekday path, which no longer exists; the class now lives under airflow.providers.standard.sensors.weekday. BaseXCom's xref in the common.io XCom backend guide pointed at airflow.models.xcom.BaseXCom, a deprecated compat shim. The task-sdk docs only expose it at the flat airflow.sdk.BaseXCom path (autoapiclass in task-sdk/docs/api.rst, autoapi_generate_api_docs=False elsewhere), matching the existing convention for this class of xref (e.g. airflow.sdk.ResumableJobMixin). The TimeSensor xref in the same sensor guide is also broken (points at a non-existent sensors.time_sensor module) but is left alone here since #69610, #69746, and #69925 are all open against that same paragraph.
5 participants
What is the change?
TimeSensor(start_from_trigger=True)now starts through a newTimeOfDayTriggerthat stores only the parse-stabletarget_timeandtzand resolves the concrete UTC moment when the trigger starts.TimeSensor.__init__builds a per-instanceStartTriggerArgsinstead of mutating the class attribute.Why did I do it?
Two defects.
__init__mutated the shared class-levelStartTriggerArgs, so every sensor advertised the last-constructed sensor's moment. And that moment came fromdatetime.now()at parse time, so each Dag-processor parse produced a different serialized blob and a new DagVersion.Fixes: #69543
How did I do it?
I added
TimeOfDayTriggerplusresolve_time_of_day_momentandserializable_timezonehelpers intriggers/temporal.py. Spring-forward gaps shift forward, ambiguous fall-back times usefold=0, andserialize()persists the resolved moment so a reconstruct after midnight does not move the wait target.execute()andpoke()resolve the moment once per attempt and cache it. A sensor with no Dag falls back to UTC when poking and raises forstart_from_trigger.What's the impact?
No more dag_version churn from repeated parses. The
start_from_triggerAPI is preserved, so no major bump ofapache-airflow-providers-standard.trigger_kwargsstays accepted and ignored.What's the test plan?
pytest providers/standard/tests/unit/standard/sensors/test_time.py -qandpytest providers/standard/tests/unit/standard/triggers/test_temporal.py -q. New tests cover serialization stability across mockednow(), midnight caching, DST spring and fall, fixed and named timezones, shared-class isolation, andend_from_triggerpropagation.